Conversation
WalkthroughRemoves legacy CI files and some documentation, renames test-suite identifiers from mono_* to eloq_* across docs and scripts, and adds ELOQSQL symlink registration and install-link entries to CMake build/install logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI script
participant Scripts as concourse / bootstrap
participant CMake as CMake (symlinks.cmake)
participant Build as build system / MYSQL_ADD_EXECUTABLE
participant Install as install step
rect rgba(200,230,255,0.4)
Dev->>Scripts: invoke tests (eloq_main/eloq_basic/eloq_multi)
Scripts->>Scripts: echo labels & run MTR with eloq_* suites
end
rect rgba(230,255,230,0.4)
CMake->>CMake: REGISTER_ELOQSQL_SYMLINK registrations
CMake->>Build: GET_ELOQSQL_SYMLINK(target)
Build->>Build: create post-build symlink / hardlink
Build->>Install: install binaries + eloqsql symlinks (component: Server)
end
note right of Install: Result: installed eloqsql-* names alongside legacy names
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/mysql_add_executable.cmake (1)
124-151: LGTM with optional refactoring suggestion.The ELOQSQL symlink creation logic correctly mirrors the existing legacy symlink handling. However, there's significant code duplication between lines 96-122 and 124-151.
Consider extracting the common symlink creation logic into a helper macro to reduce duplication:
+MACRO(CREATE_AND_INSTALL_SYMLINK target link destination comp) + IF(UNIX) + ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E create_symlink ${target} ${link} + COMMENT "Creating ${link} link" + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) + INSTALL(PROGRAMS + ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${link} + DESTINATION ${destination} + COMPONENT ${comp}) + ELSE() + SET(link ${link}.exe) + ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD + COMMAND cmake -E remove -f ${link} + COMMAND mklink /H ${link} $<TARGET_FILE_NAME:${target}> + COMMENT "Creating ${link} link" + WORKING_DIRECTORY $<TARGET_FILE_DIR:${target}>) + INSTALL(PROGRAMS $<TARGET_FILE_DIR:${target}>/${link} DESTINATION ${destination} COMPONENT ${comp}) + ENDIF() +ENDMACRO()Then both blocks could call this helper macro.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitlab-ci.yml(0 hunks)CMakeLists.txt(1 hunks)CREDITS(0 hunks)INSTALL-WIN-SOURCE(0 hunks)KNOWN_BUGS.txt(0 hunks)README.md(6 hunks)appveyor.yml(0 hunks)cmake/mysql_add_executable.cmake(1 hunks)cmake/symlinks.cmake(1 hunks)concourse/scripts/main.bash(2 hunks)concourse/scripts/main.ent.bash(2 hunks)concourse/scripts/pr.ent.bash(2 hunks)scripts/CMakeLists.txt(1 hunks)scripts/mysql_system_tables.sql(1 hunks)
💤 Files with no reviewable changes (5)
- KNOWN_BUGS.txt
- INSTALL-WIN-SOURCE
- .gitlab-ci.yml
- CREDITS
- appveyor.yml
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~198-~198: There might be a mistake here.
Context: ...ests. #### 1. eloq_basic and eloq_main: Edit eloqsql/concourse/scripts/mtr_boots...
(QB_NEW_EN)
🔇 Additional comments (12)
scripts/mysql_system_tables.sql (1)
33-33: Verify the intentional disabling of the sequences table creation.The CREATE TABLE statement for the
sequencestable (used for auto-increment ID generation) has been commented out with no explanation. This is a core functionality that could break database initialization or upstream code that depends on it.Clarify:
- Is this change intentional, or was it accidental?
- Is the sequences table still needed by the system?
- If sequences are no longer used, remove the line entirely (rather than commenting it out) with a clear rationale in the commit message.
- If sequences will be created elsewhere, document that and remove this commented code.
CMakeLists.txt (1)
553-554: LGTM! Documentation installation updated correctly.The removal of CREDITS aligns with the PR objectives, and keeping the old line commented out provides helpful context for future reference.
concourse/scripts/main.ent.bash (2)
225-226: LGTM! Test suite references updated correctly.The renaming from
mono_main,mono_basictoeloq_main,eloq_basicis consistent with the PR objectives and matches the directory renaming.
249-250: LGTM! Test suite reference updated correctly.The renaming from
mono_multitoeloq_multiis consistent with the PR objectives.concourse/scripts/pr.ent.bash (2)
246-247: LGTM! Test suite references updated correctly.Consistent with the changes in
main.ent.bash, properly updating the suite names toeloq_main,eloq_basic.
270-271: LGTM! Test suite reference updated correctly.Consistent with the changes in
main.ent.bash, properly updating the suite name toeloq_multi.concourse/scripts/main.bash (2)
214-215: LGTM! Test suite references updated correctly.Consistent renaming from
mono_main,mono_basictoeloq_main,eloq_basicacross all CI scripts.
238-239: LGTM! Test suite reference updated correctly.Consistent renaming from
mono_multitoeloq_multiacross all CI scripts.scripts/CMakeLists.txt (1)
227-227: LGTM! ELOQSQL install script symlink added correctly.The new
eloqsql-install-dbsymlink properly complements the existingmysql_install_dblink, consistent with the ELOQSQL naming scheme introduced throughout the PR.README.md (2)
198-198: LGTM! Documentation updated correctly for eloq_basic and eloq_main.All references to the renamed test suites are updated consistently throughout the section, including section headers, commands, and configuration examples.
Also applies to: 237-237, 248-248
252-252: LGTM! Documentation updated correctly for eloq_multi.All references to the renamed test suite are updated consistently, including section headers, comments, configuration examples, and commands.
Also applies to: 270-270, 307-307, 312-312, 325-325
cmake/symlinks.cmake (1)
57-81: LGTM! ELOQSQL symlink infrastructure implemented correctly.The new ELOQSQL symlink system properly mirrors the existing MariaDB symlink infrastructure:
- Includes appropriate include guards to prevent redefinition
- Defines registration and lookup macros following the same pattern
- Registers five essential ELOQSQL binary aliases
This integrates cleanly with the symlink creation logic in
cmake/mysql_add_executable.cmakeandscripts/CMakeLists.txt.
lzxddz
force-pushed
the
refactor-repo-structure
branch
from
6f3cb84 to
8269112
Compare
Remove unsed files: KNOWN_BUGS.txt/INSTALL-WIN-SOURCE/CREDITS/appveyor.yml/.gitlab-ci.yml Move my-config.cnf → scripts/ and rename to eloqsql.cnf new symbol link for eloqsql ( eloqsql -> mariadbd eloqsql-cli -> mariadb eloqsql-safe -> mariadbd-safe eloqsql-safe-helper -> mariadbd-safe-helper eloqsql-install-db -> mariadb-install-db ) Rename mysql-test/mono_basic -> mysql-test/eloq_basic Rename mysql-test/mono_main -> mysql-test/eloq_main Rename mysql-test/mono_multi -> mysql-test/eloq_multi
lzxddz
force-pushed
the
refactor-repo-structure
branch
from
8269112 to
264bc66
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmake/mysql_add_executable.cmake (1)
124-150: Consider refactoring to eliminate duplication.This ELOQSQL links block duplicates the entire legacy links logic (lines 96-122). The only difference is the symlink resolution function call.
Consider extracting the common logic:
# Helper macro to create and install links MACRO(CREATE_AND_INSTALL_LINKS target link_getter) ${link_getter}(${target} link) IF(link) IF(UNIX) ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD COMMAND ${CMAKE_COMMAND} -E create_symlink ${target} ${link} COMMENT "Creating ${link} link" WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${link} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) ELSE() SET(link ${link}.exe) ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD COMMAND cmake -E remove -f ${link} COMMAND mklink /H ${link} $<TARGET_FILE_NAME:${target}> COMMENT "Creating ${link} link" WORKING_DIRECTORY $<TARGET_FILE_DIR:${target}>) INSTALL(PROGRAMS $<TARGET_FILE_DIR:${target}>/${link} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) ENDIF() ENDIF() ENDMACRO()Then replace both blocks with:
# create MySQL named "legacy links" CREATE_AND_INSTALL_LINKS(${target} GET_SYMLINK) # create ELOQSQL named "links" CREATE_AND_INSTALL_LINKS(${target} GET_ELOQSQL_SYMLINK)cmake/symlinks.cmake (1)
57-81: Consider refactoring to eliminate system duplication.The ELOQSQL symlink system duplicates the entire MARIADB symlink infrastructure. This creates maintenance overhead when updates are needed.
Consider a parameterized approach to support multiple symlink namespaces:
MACRO(DEFINE_SYMLINK_SYSTEM prefix) if(COMMAND REGISTER_${prefix}_SYMLINK) return() endif() macro(REGISTER_${prefix}_SYMLINK from to) list(APPEND ${prefix}_SYMLINK_FROMS ${from}) list(APPEND ${prefix}_SYMLINK_TOS ${to}) endmacro() MACRO(GET_${prefix}_SYMLINK name out) set(${out}) list(FIND ${prefix}_SYMLINK_FROMS ${name} _index) if (${_index} GREATER -1) list(GET ${prefix}_SYMLINK_TOS ${_index} ${out}) endif() ENDMACRO() ENDMACRO() DEFINE_SYMLINK_SYSTEM(MARIADB) DEFINE_SYMLINK_SYSTEM(ELOQSQL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitlab-ci.yml(0 hunks)CMakeLists.txt(1 hunks)CREDITS(0 hunks)INSTALL-WIN-SOURCE(0 hunks)KNOWN_BUGS.txt(0 hunks)README.md(6 hunks)appveyor.yml(0 hunks)cmake/mysql_add_executable.cmake(1 hunks)cmake/symlinks.cmake(1 hunks)concourse/scripts/main.bash(2 hunks)concourse/scripts/main.ent.bash(2 hunks)concourse/scripts/pr.ent.bash(2 hunks)scripts/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (5)
- .gitlab-ci.yml
- appveyor.yml
- CREDITS
- KNOWN_BUGS.txt
- INSTALL-WIN-SOURCE
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- concourse/scripts/main.bash
- concourse/scripts/main.ent.bash
- concourse/scripts/pr.ent.bash
🔇 Additional comments (3)
scripts/CMakeLists.txt (1)
227-227: LGTM!The eloqsql-install-db install link is correctly added and aligns with the new ELOQSQL symlink system.
README.md (1)
198-325: LGTM!The documentation updates consistently reflect the test suite renaming from mono_* to eloq_* across all headings, commands, and configuration examples.
cmake/symlinks.cmake (1)
66-70: LGTM!The ELOQSQL symlink registrations correctly map the MariaDB binaries to their EloqSQL equivalents as specified in the PR objectives.
|
README still use mysqld, should replace with eloqsql? Directory like BUILD,Docs,extra,Debian, libmysqld could be removed? |
2 participants
Remove unsed files: KNOWN_BUGS.txt/INSTALL-WIN-SOURCE/CREDITS/appveyor.yml/.gitlab-ci.yml Move my-config.cnf → scripts/ and rename to eloqsql.cnf
new symbol link for eloqsql (
eloqsql -> mariadbd
eloqsql-cli -> mariadb
eloqsql-safe -> mariadbd-safe
eloqsql-safe-helper -> mariadbd-safe-helper
eloqsql-install-db -> mariadb-install-db )
Rename mysql-test/mono_basic -> mysql-test/eloq_basic Rename mysql-test/mono_main -> mysql-test/eloq_main Rename mysql-test/mono_multi -> mysql-test/eloq_multi
Summary by CodeRabbit
Chores
Documentation
New Features
Refactor